Handled multiple sessions for one context#15
Merged
Conversation
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
from
1dd9b24 to
f891418
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-session support: new utility to collect sessions for a context UID, refactors views and templates to render multiple sessions, updates tests to cover multi-session viewlet behavior, and records the change in CHANGES.rst. Changes
Sequence DiagramsequenceDiagram
participant Context as Context Item
participant Viewlet as ItemSessionInfoViewlet
participant Utils as get_sessions_for()
participant Template as session_info.pt
participant Render as Rendered Output
Context->>Viewlet: render request
Viewlet->>Viewlet: sessions() called
Viewlet->>Utils: get_sessions_for(context_uid)
Utils->>Utils: read annotations, map c_uids → session IDs, dedupe
Utils-->>Viewlet: return sessions list
Viewlet->>Template: pass sessions list
Template->>Template: tal:repeat over sessions
Template-->>Render: output per-session HTML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/imio/esign/browser/templates/session_info.pt (1)
39-39: Scope the odd/even JS selector to the current session.
With multiple sessions rendered, each script run reprocesses all tables on the page. Scoping to the current session avoids redundant work.♻️ Proposed tweak
- <script>$('table.context_viewlet_signers_table').each(setoddeven);</script> + <script tal:content="structure string:$('#collapsible-session-info-${session_id} table.context_viewlet_signers_table').each(setoddeven);"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/templates/session_info.pt` at line 39, The script currently selects all tables via $('table.context_viewlet_signers_table').each(setoddeven) which reprocesses tables across all sessions; scope the selector to the current session container instead (e.g., use the template's root element or a session-specific wrapper) and call .find('table.context_viewlet_signers_table').each(setoddeven) or the equivalent jQuery context so setoddeven only runs on tables inside the current session element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/browser/views.py`:
- Around line 259-266: The loop that builds session dicts from annot entries
(variables: annot, f_uid, session_id, seen, session, result) can create empty
sessions when annot["uids"] points to a session_id removed from
annot["sessions"], causing template KeyErrors; change the logic in the loop that
iterates over annot["c_uids"].get(self.context.UID(), []) to first check that
session_id exists in annot["sessions"] (e.g., if session_id in
annot["sessions"]) and skip (continue) if it does not, so you only build and
append session = dict(annot["sessions"][session_id]) and set session["id"] when
the session data is present and valid.
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 226-255: The test assumes session ids 0 and 1 which makes it
brittle; update test_sessions_multiple_sessions to capture the ids returned by
add_files_to_session for each call (use the returned session id(s) rather than
hard-coding {0, 1}), then assert against that set when checking session_ids and
when validating rendered_ids and the strings produced by the
mock_index/viewlet.render; locate references to add_files_to_session,
ItemSessionInfoViewlet, viewlet.sessions, mock_index, viewlet.index and
viewlet.render to replace hard-coded ids with the variables holding the created
session ids.
---
Nitpick comments:
In `@src/imio/esign/browser/templates/session_info.pt`:
- Line 39: The script currently selects all tables via
$('table.context_viewlet_signers_table').each(setoddeven) which reprocesses
tables across all sessions; scope the selector to the current session container
instead (e.g., use the template's root element or a session-specific wrapper)
and call .find('table.context_viewlet_signers_table').each(setoddeven) or the
equivalent jQuery context so setoddeven only runs on tables inside the current
session element.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.rstsrc/imio/esign/browser/templates/macros.ptsrc/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/tests/test_browser_views.py
sgeulette
requested changes
Mar 6, 2026
Member
There was a problem hiding this comment.
ok pour l'approche mais il faudrait avoir un seul collapsible qui ouvre la liste des tables d'infos sur les sessions.
Ordonner également par ordre inverse car c'est l'approche dans la liste des sessions aussi.
Tu peux aussi ajouter des classes odd/even pour styler les sessions et les distinguer plus facilement
gbastien
reviewed
Mar 9, 2026
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
from
f891418 to
9c16952
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/esign/tests/test_browser_views.py (1)
406-406: Minor inconsistency: request access pattern differs from other test classes.This test uses
self.portal.REQUESTwhile_BaseSessionViewTestusesself.layer["request"]. Consider aligning for consistency.♻️ Optional consistency fix
- self.request = self.portal.REQUEST + self.request = self.layer["request"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_browser_views.py` at line 406, The test is accessing the request via self.portal.REQUEST which is inconsistent with other tests that use the test layer's request; update the assignment in this test to use the layer request (self.layer["request"]) instead of self.portal.REQUEST so it matches _BaseSessionViewTest's access pattern (replace references to self.portal.REQUEST with self.layer["request"] and ensure any subsequent uses of self.request rely on that layer request).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Line 406: The test is accessing the request via self.portal.REQUEST which is
inconsistent with other tests that use the test layer's request; update the
assignment in this test to use the layer request (self.layer["request"]) instead
of self.portal.REQUEST so it matches _BaseSessionViewTest's access pattern
(replace references to self.portal.REQUEST with self.layer["request"] and ensure
any subsequent uses of self.request rely on that layer request).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7277cbed-75b7-4884-b454-69510903f7b6
📒 Files selected for processing (5)
CHANGES.rstsrc/imio/esign/browser/templates/macros.ptsrc/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/tests/test_browser_views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.rst
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
2 times, most recently
from
3f7d3f5 to
5506f92
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/imio/esign/browser/templates/session_info.pt (1)
16-37:⚠️ Potential issue | 🟠 MajorThe table hook is broken after the rename, and the repeated ids are now invalid HTML.
Line 29 reuses
context_viewlet_session_table, but Line 37 still targetscontext_viewlet_signers_table. On top of that,tal:repeatnow duplicates the same table id for every rendered session. Use a shared class or data attribute for these tables and update the selector to match.Suggested direction
- <table id="context_viewlet_session_table" class="no-style-table table-view-widgets"> + <table class="no-style-table table-view-widgets js-session-table"> ... - <table id="context_viewlet_session_table" class="no-style-table table-view-widgets"> + <table class="no-style-table table-view-widgets js-session-table"> ... - <script>$('table#context_viewlet_signers_table').each(setoddeven);</script> + <script>$('.js-session-table').each(setoddeven);</script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/templates/session_info.pt` around lines 16 - 37, The template duplicates the same id context_viewlet_session_table for each session and the JS still selects table#context_viewlet_signers_table, breaking the hook; replace the id on both table elements with a shared class or data attribute (e.g. class="context-viewlet-session-table" or data-table="session") inside the tal:repeat blocks (keep tal:repeat and template.macros usage in view.get_table_rows(1/2) unchanged), and update the script selector to target the new class/data attribute (for example $('table.context-viewlet-session-table').each(setoddeven);) so the selector matches the rendered tables and avoids duplicate ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/imio/esign/browser/templates/session_info.pt`:
- Around line 16-37: The template duplicates the same id
context_viewlet_session_table for each session and the JS still selects
table#context_viewlet_signers_table, breaking the hook; replace the id on both
table elements with a shared class or data attribute (e.g.
class="context-viewlet-session-table" or data-table="session") inside the
tal:repeat blocks (keep tal:repeat and template.macros usage in
view.get_table_rows(1/2) unchanged), and update the script selector to target
the new class/data attribute (for example
$('table.context-viewlet-session-table').each(setoddeven);) so the selector
matches the rendered tables and avoids duplicate ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dad0479-5e7d-404d-8527-de6d811cee77
📒 Files selected for processing (3)
src/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/browser/views.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/utils.py`:
- Around line 522-535: get_sessions_for currently returns session dicts from
annot["sessions"] without their session_id, and when readonly=True the deepcopy
loses any external handle; fix by attaching the session id into each session
mapping before returning: when you fetch session = annot["sessions"][session_id]
inside get_sessions_for, create a new mapping that includes all session fields
plus "id": session_id (e.g., session_with_id = dict(session, id=session_id)) and
append that to result so both readonly and non-readonly returns include the "id"
key; use deepcopy(result) only for the readonly branch as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47486412-d8d4-41d6-b692-b16929462c74
📒 Files selected for processing (3)
src/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/browser/templates/session_info.pt
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
from
5506f92 to
c43fd60
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/imio/esign/browser/views.py (1)
216-228:⚠️ Potential issue | 🟡 MinorType inconsistency for
session["id"].The
session_idfrom the request form is a string, but it's assigned directly tosession["id"]on line 227. In contrast,get_sessions_forin utils.py assigns the integersession_idtosession["id"]. This inconsistency could cause comparison issues when templates or downstream code compare session IDs.🔧 Proposed fix
`@property` def sessions(self): - session = None session_id = self.request.form.get("esign_session_id[]", None) if not session_id: - return + return [] + session_id = int(session_id) sessions = get_session_annotation()["sessions"] - session = sessions.get(int(session_id)) + session = sessions.get(session_id) if not session: - return + return [] session = deepcopy(session) session["id"] = session_id return [session]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/views.py` around lines 216 - 228, The sessions property assigns the string request form value to session["id"], causing a type mismatch with get_sessions_for which uses an int; convert the form session_id to an integer before assigning (e.g., use int(session_id) when setting session["id"] and when fetching from sessions) so session IDs are consistently ints across the sessions property and get_sessions_for in utils.py.src/imio/esign/browser/templates/session_info.pt (1)
37-37:⚠️ Potential issue | 🟠 MajorJavaScript selector no longer matches any elements.
The script references
table#context_viewlet_signers_table, but the table IDs have been changed tocontext_viewlet_session_signers_${session/id}. This selector won't match any elements, sosetoddevenwill never be applied.Additionally, this script is inside the
tal:repeatloop, so it will be rendered once per session, which is unnecessary.🔧 Proposed fix
Move the script outside the repeat loop and update the selector to match the new dynamic IDs:
</div> - <script>$('table#context_viewlet_signers_table').each(setoddeven);</script> </div> + <script>$('table[id^="context_viewlet_session_signers_"]').each(setoddeven);</script> </div>This uses an attribute-starts-with selector to match all session signer tables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/templates/session_info.pt` at line 37, The inline script currently inside the tal:repeat references table#context_viewlet_signers_table which no longer exists so setoddeven never runs; move the <script> out of the tal:repeat block so it is rendered once per page and change the selector to target the new dynamic IDs (use an attribute-starts-with selector like [id^="context_viewlet_session_signers_"] or similar) so setoddeven is applied to all session signer tables (ensure you keep the function name setoddeven unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/imio/esign/browser/templates/session_info.pt`:
- Line 37: The inline script currently inside the tal:repeat references
table#context_viewlet_signers_table which no longer exists so setoddeven never
runs; move the <script> out of the tal:repeat block so it is rendered once per
page and change the selector to target the new dynamic IDs (use an
attribute-starts-with selector like [id^="context_viewlet_session_signers_"] or
similar) so setoddeven is applied to all session signer tables (ensure you keep
the function name setoddeven unchanged).
In `@src/imio/esign/browser/views.py`:
- Around line 216-228: The sessions property assigns the string request form
value to session["id"], causing a type mismatch with get_sessions_for which uses
an int; convert the form session_id to an integer before assigning (e.g., use
int(session_id) when setting session["id"] and when fetching from sessions) so
session IDs are consistently ints across the sessions property and
get_sessions_for in utils.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 959ac3e3-0f9a-4281-bc22-b39d15b4d217
📒 Files selected for processing (3)
src/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/utils.py
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
from
c43fd60 to
e44edf8
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/imio/esign/tests/test_browser_views.py (1)
468-476: Add one render-level assertion for the multi-session path.This currently proves
viewlet.sessions, but not the template changes insession_info.pt. A regression that renders only one block would still pass here.Proposed test extension
def test_sessions_multiple_sessions(self): """Files in two sessions (different discriminators) → sessions returns two dicts.""" add_files_to_session(self.signers, [self.annexes[0].UID()], discriminators=("a",)) add_files_to_session(self.signers, [self.annexes[1].UID()], discriminators=("b",)) viewlet = ItemSessionInfoViewlet(self.folder, self.request, None, None) sessions = viewlet.sessions self.assertEqual(len(sessions), 2) session_ids = {s["id"] for s in sessions} self.assertEqual(session_ids, {0, 1}) + rendered = viewlet.render() + self.assertIn("context_viewlet_session_table_0", rendered) + self.assertIn("context_viewlet_session_table_1", rendered) + self.assertIn("context_viewlet_session_signers_0", rendered) + self.assertIn("context_viewlet_session_signers_1", rendered)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_browser_views.py` around lines 468 - 476, Extend test_sessions_multiple_sessions to assert the rendered template output contains both session blocks: call the viewlet's render method (e.g., ItemSessionInfoViewlet(self.folder, self.request, None, None).render() or __call__()) and assert that the returned HTML includes markers for both sessions (for example the two discriminators "a" and "b" or the two session ids 0 and 1, or a repeated session-block CSS/class used in session_info.pt). This ensures the template actually renders both sessions, not just that viewlet.sessions contains two entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/browser/views.py`:
- Around line 217-228: The sessions view should parse "esign_session_id[]"
defensively: replace the direct request.form.get + int(session_id) in sessions()
with logic that reads all values (e.g., request.form.getlist / getall), picks
the first non-empty scalar, and wraps int(...) in a try/except (catch
ValueError/TypeError) to fall back to returning [] on bad input; then proceed to
look up the session in get_session_annotation()["sessions"], deepcopy it, set
session["id"]=session_id and return [session] only when a valid integer id and
session exist.
---
Nitpick comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 468-476: Extend test_sessions_multiple_sessions to assert the
rendered template output contains both session blocks: call the viewlet's render
method (e.g., ItemSessionInfoViewlet(self.folder, self.request, None,
None).render() or __call__()) and assert that the returned HTML includes markers
for both sessions (for example the two discriminators "a" and "b" or the two
session ids 0 and 1, or a repeated session-block CSS/class used in
session_info.pt). This ensures the template actually renders both sessions, not
just that viewlet.sessions contains two entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 562de1a3-4ccf-4b24-9ce5-cfe22a3b198d
📒 Files selected for processing (5)
src/imio/esign/browser/templates/macros.ptsrc/imio/esign/browser/templates/session_info.ptsrc/imio/esign/browser/views.pysrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/utils.py
chris-adam
force-pushed
the
PARAF-345/1_context_multiple_sessions
branch
from
e44edf8 to
81c19e5
Compare
chris-adam
commented
Mar 10, 2026
src/imio/esign/utils.py
Outdated
| session = annot["sessions"][session_id] | ||
| if readonly: | ||
| session = deepcopy(session) | ||
| session["id"] = session_id |
Contributor
Author
There was a problem hiding this comment.
Ici, je ne suis pas sûr de l'implémentation. J'ai besoin de l'ID dans la session mais je n'ai pas envie de modifier l'annotation. je fais donc un deepcopy pour chaque session, et il y a une différence dans l'output si on est en readonly ou non
…o matter readonly=True or False Adapted template displaying it, also to display multiple sessions using odd/even Added test_get_sessions_for
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / UI
Tests
Chores